Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

if results too large get each result one at a time #1118

Closed
wants to merge 13 commits into from

Conversation

bjartek
Copy link
Collaborator

@bjartek bjartek commented Jul 4, 2023

Could not find a test for the original method so i did not add one here.

Closes #1107

…ach transaction result at a time and append it to the array
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2023

Codecov Report

Merging #1118 (78eabac) into master (daa1550) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1118   +/-   ##
=======================================
  Coverage   39.28%   39.28%           
=======================================
  Files          37       37           
  Lines        1876     1876           
=======================================
  Hits          737      737           
  Misses       1051     1051           
  Partials       88       88           
Flag Coverage Δ
unittests 39.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just one minor comment

flowkit/flowkit.go Outdated Show resolved Hide resolved
…: code = ResourceExhausted desc = grpc: trying to send message larger than max (26319669 vs. 20971520)
@bjartek
Copy link
Collaborator Author

bjartek commented Jul 11, 2023

Question: will this not hit rate limit really fast? The blocks where this hits have hundreds of transactions and that will result in a lot of hits against the grpc endpoint.

@janezpodhostnik
Copy link
Contributor

The gateway should take care of handling rate limits I think. So basically outgoing calls would automatically be delayed if they are getting close to the rate limit. I did something like that here previously https://github.com/onflow/flow-batch-scan/blob/main/client/interceptors/uci_rate_limit.go

@bjartek
Copy link
Collaborator Author

bjartek commented Jul 17, 2023

I just figured out that this will not work for some scenarios. Basically the only way atm to get SystemChunkTransactionResults is through the method that hits the limits.

My last commit could possible be ignored since i was confused by the changed systemChunkTransactionId

@bjartek
Copy link
Collaborator Author

bjartek commented Jul 17, 2023

Block id 55179362 is a block that has systemChunkTransactions and it is not possible to get all transactionResults for that block since it is too large, how do i then get the systemChunkEvents?

@bjartek
Copy link
Collaborator Author

bjartek commented Jul 17, 2023

Ammended the pr with using the new GetTransactionByHeight that i exposed in another PR.

flowkit/go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
}
errorMessage := err.Error()

if strings.Contains(errorMessage, "received message larger than max") || strings.Contains(errorMessage, "trying to send message larger than max") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have access to these errors? so we could import them by type instead of hardcoding error message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they look like standard GRPC error messages to me, so i doubt it.

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to handle this, left some comments

@bjartek
Copy link
Collaborator Author

bjartek commented Jul 28, 2023

I pushed changes but they do not appear here

@bjartek bjartek mentioned this pull request Aug 1, 2023
@bjartek
Copy link
Collaborator Author

bjartek commented Aug 21, 2023

We have decided to not do this, it should be done on another level.

@bjartek bjartek closed this Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When fetching Transasctions and Results for a block that is very large it fails
5 participants